kie-issues#1298: Decision Services & multiple DRDs: make it possible o add external Decisions to a Decision Service #2508

danielzhe · 2024-08-02T22:07:57Z

Closes: apache/incubator-kie-issues#1298

ds.mp4

…to add external Decisions to a Decision Service.

danielzhe · 2024-08-02T22:08:42Z

More testes for you, @ljmotta 😄

danielzhe · 2024-08-02T22:09:12Z

packages/dmn-editor/stories/dev/DevWebApp.stories.tsx

 import { useCallback, useMemo, useRef, useState } from "react";
+import type { Meta, StoryObj } from "@storybook/react";


Those changes are made automatically by the Prettier

I think Prettier doesn't change the imports... Maybe it's your VS Code "Organize imports"?

tiagobento

Thanks for the PR @danielzhe! Made some change requests inline, please consider them.

tiagobento · 2024-08-05T14:58:24Z

packages/dmn-editor/src/mutations/addDecisionToDecisionService.ts

 }: {
  definitions: Normalized<DMN15__tDefinitions>;
-  decisionId: string;
+  drgElement: DrgElement;
  decisionServiceId: string;
  drdIndex: number;
  snapGrid: SnapGrid;
+  decisionShape: Normalized<DMNDI15__DMNShape>;
+  elementId: string;
 }) {


elementId --> decisionId

I understand drgElement doesn't necessarily is inside definitions, so please make this action dependent on external models too. There's no way to know whether or not drgElement and elementId are aligned...

In the previous code we was getting the ID, in Diagram.tsx using the following code:

decisionId: selectedNodes[i].data.dmnObject!["@_id"]!, // We can assume that all selected nodes are Decisions because the contaiment was validated above.

So I think it is safe to assume that selectedNodes[i].data.dmnObject is always a Decision which is the DrgElement that we looking for. Maybe I should change the type of DrgElement I defined here
to:

export type DecisionDrgElement = Normalized<{ __$$element: "decision" } & DMN15__tDecision>

About the ID, I can't see any case where the ID of the selected node will be different than the DRG, except for the imported nodes the ID in the DRG is not full, it is only the local ID (the Guid), but from selectedNodes[I].["@_id"] is the real ID used in the current diagram (namespace + id).

In fact, we don't have DRG element always in the definitions: the external nodes do not have DRG element, only DMNDI. The DRG we have in selectedNodes[i].data.dmnObject is a loaded DRG from the external model, that's why its ID is different to the internal one, the one in href.
See below:

<decisionService name="New Decision Service" id="_A0908B2B-DBE7-41C7-B055-0C0DAD17CE54"> <variable name="New Decision Service" id="_145CD7A7-B14D-4D63-B9D3-85601FC915B1" /> <outputDecision href="https://kie.apache.org/dmn/_D19B0015-2CBD-4BA8-84A9-5F554D84A9E1#_05621ED4-9236-47F1-B93A-164A4527B136" /> <encapsulatedDecision href="https://kie.apache.org/dmn/_D19B0015-2CBD-4BA8-84A9-5F554D84A9E1#_1991FB34-1253-4A54-AD3D-89697938DDFA" /> <encapsulatedDecision href="#_8E89DDF6-7ABC-4FBF-A1B6-B68BC32A52AE" /> </decisionService> <decision name="New Decision" id="_8E89DDF6-7ABC-4FBF-A1B6-B68BC32A52AE"> <variable name="New Decision" id="_BFB8A43B-5ADA-47D8-815E-8A0505814AA1" /> </decision>

Notice only the 8E89DDF6-7ABC-4FBF-A1B6-B68BC32A52AE is present in the file.
The other nodes are not, only as a dmndi.

We only need the ID, but we need to know that this ID represents a Decision. The only way to know that is having the externalModelsByNamespace passed as parameter to the "addDecisionToDecisionService" mutation, so we can make that validation. You're mentioning the arguments you're passing when calling this mutation, but the mutation itself, after your changes, can be called with

{ elementId: "bar" drgElement: { "@_id": "foo" ... } }

which has the potential of being wrong. What I'm saying is that we rollback to the way it was before, and we include the externalModelsByNamespace parameters so we can look if that href (nmspc#id) represents a Decision on the model with namespace nmspc

I think got your point! I guarantee in the Diagram.tsx that the ID represents a decision, but I am not doing the same inside the mutation, in other words, the mutation is not "safe" anymore, right?

Yes, you can guarantee that you're calling it correctly now, but you can't guarantee that this will be true forever :P

tiagobento · 2024-08-05T15:00:16Z

packages/dmn-editor/src/diagram/Palette.tsx

+            <div
+              className={"kie-dmn-editor--palette-nodes-popover"}
+              style={{ maxHeight }}
+              data-testid={"kie-tools--dmn-editor--external-nodes-container"}
+            >


The "DRG nodes" panel doesn't have a data-testid, why don't we use the same strategy we're using for it?

Do we have tests for "DRG Nodes" panel? I only found for drag nodes from the palette not from the "DRG Nodes" panel. 🤔

@tiagobento actually we are about to add data-testid also for the DRG Nodes panel, see the PR #2462

One thing we could unify is the data-testid suffix. @danielzhe is using -container while me is using -popover, what do you prefer? I chose -popover due to related className.

All good then! Thanks for the insights. I have no preference for the suffix, just please coordinate that it is consistent. @jomarko @danielzhe

I'll change it to -popover since it is related to the className and makes more sense! Thank you, @jomarko

tiagobento · 2024-08-05T15:00:46Z

packages/dmn-editor/src/diagram/Diagram.tsx

+                  decisionShape: selectedNodes[i].data.shape,
+                  elementId: selectedNodes[i].id, // The "real" id is here, which can be the local id or an imported node id (uri + external element id).


Please see my comment about the mutation itself...

tiagobento · 2024-08-05T15:04:45Z

packages/dmn-editor/tests-e2e/drgElements/addIncludedNodes.spec.ts

Can we rename this to externalNodes? I mean, this is the first test we have on that "domain".

tiagobento · 2024-08-05T15:06:20Z

packages/dmn-editor/tests-e2e/drgElements/addIncludedNodes.spec.ts

+    await editor.changeTab({ tab: TabName.EDITOR });
+    await palette.dragNewNode({ type: NodeType.DECISION_SERVICE, targetPosition: { x: 400, y: 200 } });
+
+    await palette.getExternalNodesButton().click();


I think this is leaking internal details... I would expect something like palette.openExternalNodesPanel().

tiagobento · 2024-08-05T15:07:39Z

packages/dmn-editor/tests-e2e/drgElements/addIncludedNodes.spec.ts

+  test("add to a Decision Service", async ({ editor, page, palette, diagram, nodes, includedModels }) => {
+    await editor.changeTab({ tab: TabName.INCLUDED_MODELS });
+
+    await includedModels.getIncludeModelButton().click();


I think this is leaking internal details... I would expect something like includedModels.addNew(). Not sure how this is being done in other parts of our test suite, though.

I did as you suggested at first, but then I found in other tests that we were using this way, getting the button and then calling the click(), like here, for example, and changed to this "clicky" approach. I will change it back. 😄

Yeah, I like it better when the actual screen elements are not exposed, to be honest, since that's how we're doing for the other parts of our tests.

tiagobento · 2024-08-05T15:08:18Z

packages/dmn-editor/tests-e2e/drgElements/addIncludedNodes.spec.ts

+    });
+
+    // We click on the button again to close the external nodes panel.
+    await palette.getExternalNodesButton().click();


Same? Maybe instead of openExternalNodesPanel you could call it toggleExternalNodesPanel

tiagobento · 2024-08-05T15:08:37Z

packages/dmn-editor/tests-e2e/drgElements/addIncludedNodes.spec.ts

+    // We move it to make it sure that the nodes are added inside the Decision Services
+    // and are really attached to it, not only "visually over it".
+    await nodes.move({ name: DefaultNodeName.DECISION_SERVICE, targetPosition: { x: 120, y: 120 } });


Great comment.

tiagobento · 2024-08-05T15:08:43Z

packages/dmn-editor/tests-e2e/drgElements/addIncludedNodes.spec.ts

+
+    await expect(diagram.get()).toHaveScreenshot("add-included-node-inside-decision-service.png");
+  });
+});


Please add more tests to the basic operations, like changing from one DS to another. Removing an external Decision from a DS, things like that.

tiagobento · 2024-08-05T15:10:20Z

packages/dmn-editor/tests-e2e/__fixtures__/nodes.ts

+      // Decision Services only have some draggable areas near the borders.
+      // If you drag it to the center, you'll drag the divide line.
+      // Also, neither the entire upper area nor the entire downer area is draggable.
+      await this.get({ name: args.name }).dragTo(this.diagram.get(), {
+        targetPosition: args.targetPosition,
+        sourcePosition: { x: 20, y: 20 },
+      });


Great comment also.

Unfortunately, we have one more problem, that node.move() can be invoked also for a decision service node, that is already renamed and is completely different from DefaultNodeName.DECISION_SERVICE

I think we should also document that the decision service move will work only for 'default name' decision service nodes.

Good point @jomarko. @danielzhe I think we can update this to have a flag "grabAtBorders" that we use as true when moving Decision Services around.

I made a recent change where the name must now contain DefaultNodeName.DECISION_SERVICE, but not exactly it. But I agree, it isn't good to rely on the name. What we can do? A separate "move" for Decision Service or add a doc in the code?

We also can just change the move to always get the node from a corner not from the middle. Then it will work for every node. Can add an optional parameter where the user can add the sourcePosition also. I think this is the best option. What do you guys think? @jomarko @tiagobento

public async move(args: { grabAtBorder?: boolean; targetPosition: { x: number; y: number } }) { if (args.grabAtBorder ?? false) { // Decision Services only have some draggable areas near the borders. // If you drag it to the center, you'll drag the divide line. // Also, neither the entire upper area nor the entire downer area is draggable. await this.get({ name: args.name }).dragTo(this.diagram.get(), { targetPosition: args.targetPosition, sourcePosition: { x: 20, y: 20 }, }); } else { await this.get({ name: args.name }).dragTo(this.diagram.get(), { targetPosition: args.targetPosition, }); } }

jomarko · 2024-08-05T15:39:08Z

I will review this tomorrow.

… outside.

jomarko

Some comments inline, however manual check passed and the feature works well!

jomarko · 2024-08-06T06:29:47Z

packages/dmn-editor/src/externalNodes/ExternalNodesPanel.tsx

@@ -159,6 +159,7 @@ export function ExternalNodesPanel() {
                          externalDrgElementId: drgElement["@_id"]!,
                        })
                      }
+                      data-testid={`kie-tools--dmn-editor--external-node-${_import["@_name"]}-${drgElement["@_name"]}`}


Is ${_import["@_name"]} always non empty? I mean containing some non white characters?

No, it can be an empty string. Great catch.

jomarko · 2024-08-06T06:42:32Z

packages/dmn-editor/tests-e2e/__fixtures__/nodes.ts

+      // Decision Services only have some draggable areas near the borders.
+      // If you drag it to the center, you'll drag the divide line.
+      // Also, neither the entire upper area nor the entire downer area is draggable.
+      await this.get({ name: args.name }).dragTo(this.diagram.get(), {
+        targetPosition: args.targetPosition,
+        sourcePosition: { x: 20, y: 20 },
+      });


Unfortunately, we have one more problem, that node.move() can be invoked also for a decision service node, that is already renamed and is completely different from DefaultNodeName.DECISION_SERVICE

I think we should also document that the decision service move will work only for 'default name' decision service nodes.

jomarko · 2024-08-06T06:48:17Z

packages/dmn-editor/tests-e2e/__fixtures__/palette.ts

+  public async dragExternalNode(args: {
+    includedModelName: string;
+    nodeName: string;
+    targetPosition: { x: number; y: number };
+  }) {
+    await this.page
+      .getByTestId("kie-tools--dmn-editor--external-nodes-container")
+      .getByTestId(`kie-tools--dmn-editor--external-node-${args.includedModelName}-${args.nodeName}`)
+      .dragTo(this.diagram.get(), { targetPosition: args.targetPosition });
+  }
+
+  public async toggleExternalNodesPanel() {
+    await this.page.getByRole("button", { name: "External nodes" }).click();
+  }


In my PR #2462, I added similar as separate fixture drgNodes.ts. Should I also add add it into palette.ts instead of a separate fixture file?

I moved to palette because we understand the "palette" as all those buttons on the left of the screen. So I think the toggle should be done in the palette.

jomarko · 2024-08-06T06:54:01Z

...shots__/Google-Chrome/drgElements/move-external-decision-out-from-decision-service-after.png

Maybe it is not so clear , if the MY_MODEL_Diff is really out of the decision service node. Could we maybe move it a little bit more to make it clear? Or is this something like corner case test?

Agree, this screenshot is kind of ambiguous...

jomarko · 2024-08-06T06:58:24Z

packages/dmn-editor/tests-e2e/drgElements/addExternalNode.spec.ts

+    await includedModels.fillModelToInclude({ modelName: "sumDiffDs.dmn" });
+    await includedModels.selectModel({ modelName: "sumDiffDs.dmn" });


Why both fillModelToInclude and selectModel are needed please?

One "types" the string, and the other picks it from the list.

data-testid renamed. General code review. Typo fix.

danielzhe · 2024-08-06T21:03:47Z

packages/dmn-editor/src/diagram/Diagram.tsx

@@ -992,8 +992,9 @@ export const Diagram = React.forwardRef<DiagramRef, { container: React.RefObject
                for (let i = 0; i < selectedNodes.length; i++) {
                  deleteDecisionFromDecisionService({
                    definitions: state.dmn.model.definitions,
-                    decisionId: selectedNodes[i].data.dmnObject!["@_id"]!, // We can assume that all selected nodes are Decisions because the contaiment was validated above.
+                    decisionId: selectedNodes[i].id, // We can assume that all selected nodes are Decisions because the contaiment was validated above.


This is the full id. The ID of the selectedNodes[i].data.dmnObject!["@_id"]! does not contain the href to the external model, only the "internal" id, so inside the mutation we would have to look everywhere looking for where is the Decision.

"Full ID" = href

href = namespace#xsd:ID

xsd:ID = typically the "@_id" attribute of objects

namespace = definitions["@_namespace"]

packages/dmn-editor/src/externalNodes/ExternalNodesPanel.tsx

danielzhe · 2024-08-06T21:05:56Z

packages/dmn-editor/src/mutations/addDecisionToDecisionService.ts

-  const decision = definitions.drgElement?.find((s) => s["@_id"] === decisionId);
-  if (decision?.__$$element !== "decision") {
-    throw new Error(`DMN MUTATION: DRG Element with id '${decisionId}' is either not a Decision or doesn't exist.`);
+  const href = parseXmlHref(decisionId);


So decisionId can be in any format here:
#id
uri#id
id

But, if it is an external node, we only be able to find it if the full id is passed: uri#id

Good point. I think we better rename decisionId to decisionHref.

...and the id of a node is an href.... please see apache/incubator-kie-issues#983.... :x

danielzhe · 2024-08-06T21:07:33Z

@tiagobento @jomarko Changes applied! Ready for you to continue.

Fixing tests.

tiagobento

Thanks @danielzhe! This is looking great. Few last comments from my side.

packages/dmn-editor/src/externalNodes/ExternalNodesPanel.tsx

packages/dmn-editor/src/diagram/Diagram.tsx

tiagobento · 2024-08-07T20:16:06Z

packages/dmn-editor/src/mutations/addDecisionToDecisionService.ts

+    decisionService.encapsulatedDecision.push({ "@_href": `${hrefString}` });
  } else if (section === "output") {
    decisionService.outputDecision ??= [];
-    decisionService.outputDecision.push({ "@_href": `#${decisionId}` });
+    decisionService.outputDecision.push({ "@_href": `${hrefString}` });


tiagobento · 2024-08-07T20:16:38Z

packages/dmn-editor/src/mutations/deleteDecisionFromDecisionService.ts

+  const xmlHref = buildXmlHref({ namespace: href.namespace, id: href.id });
+  decisionService.outputDecision = (decisionService.outputDecision ?? []).filter((s) => s["@_href"] !== `${xmlHref}`);
  decisionService.encapsulatedDecision = (decisionService.encapsulatedDecision ?? []).filter(
-    (s) => s["@_href"] !== `#${decisionId}`
+    (s) => s["@_href"] !== `${xmlHref}`
  );


tiagobento · 2024-08-07T20:17:03Z

packages/dmn-editor/stories/dev/DevWebApp.stories.tsx

 import { useCallback, useMemo, useRef, useState } from "react";
+import type { Meta, StoryObj } from "@storybook/react";


I think Prettier doesn't change the imports... Maybe it's your VS Code "Organize imports"?

packages/dmn-editor/src/mutations/addDecisionToDecisionService.ts

tiagobento · 2024-08-07T20:19:51Z

packages/dmn-editor/src/mutations/addDecisionToDecisionService.ts

+    const externalDrgs = (externalModel.model as Normalized<DmnLatestModel>).definitions.drgElement;
+    const decision = externalDrgs?.find((drgElement) => drgElement["@_id"] === href.id);


Personal preference, but I always think we should avoid intermediate variables to avoid giving it a name. It's one more thing to hold onto my head when reading code...

This is a very good topic.
I like to split in variables because I find it easier to "think" step-by-step and have less complexity per line.

Also, in this case, I'm giving "meaning" to what (externalModel.model as Normalized<DmnLatestModel>).definitions.drgElement: it is the "external drgs". So I don't have to care where it comes from in line 59 or what it is. It is the "external drgs that come from somewhere". Ok, "somewhere" is just a line above, but in my mind, looking at line 59, it is a "problem solved" that I don't have to care about anymore.

But the main point is that if someone new looks at the code it can easily get what (externalModel.model as Normalized<DmnLatestModel>).definitions.drgElement means.

packages/dmn-editor/src/mutations/deleteDecisionFromDecisionService.ts

tiagobento · 2024-08-07T20:20:23Z

packages/dmn-editor/src/mutations/deleteDecisionFromDecisionService.ts

+    const externalDrgs = (externalModel.model as Normalized<DmnLatestModel>).definitions.drgElement;
+    const decision = externalDrgs?.find((drgElement) => drgElement["@_id"] === href.id);


Also same as above.

Co-authored-by: Tiago Bento <[email protected]>

…rvice.ts Co-authored-by: Tiago Bento <[email protected]>

danielzhe · 2024-09-06T15:41:47Z

@tiagobento Changes applied!

jomarko · 2024-09-09T09:15:49Z

Sorry, I get lost in the conversation, since my approval it seems more code landed in. Should I do a re-reiew?

ljmotta

I've made some comments inline. One tip regarding objects. When the object property and value has the same name you could short it just the property name:

drgElementsByNamespace: drgElementsByNamespace,
externalModelsByNamespace: externalModelsByNamespace,

drgElementsByNamespace,
externalModelsByNamespace,

Also, all test methods are with the page in their signatures, but none use it.

Example:
async ({ editor, page, palette, diagram, nodes, includedModels })

ljmotta · 2024-09-09T12:32:42Z

packages/dmn-editor/src/mutations/addDecisionToDecisionService.ts

@@ -54,8 +77,11 @@ export function addDecisionToDecisionService({
  }

  const diagram = addOrGetDrd({ definitions, drdIndex });
+  const hrefString = buildXmlHref({ namespace: href.namespace, id: href.id });


The decisionHref isn't the same as the hrefString?

No. The decisionHref can be something like uri#id, id, or even #id.

The buildXmlHref makes sure that we have uri#id or #id

A href will always have the the # on its structure. If it's possible to have a href variable without it (an id value), the variable is wrongly named. Now, looking at the source of this value, it comes from the computeDiagramData.ts file which creates the selectedNodesById map. The map id is a value made with the buildXmlHref, so all ids are href, meaning the decisionHref is correctly named.

I think this is a problem in our codebase, where we mix the id and the href, and at some point we need to address this. FYI @tiagobento

@danielzhe @tiagobento how are we going to proceed here?

@danielzhe Please use decisionHref where hrefString is currently being used, since they are exactly the same string.

ljmotta · 2024-09-09T12:40:17Z

packages/dmn-editor/src/mutations/deleteDecisionFromDecisionService.ts

-  decisionService.outputDecision = (decisionService.outputDecision ?? []).filter(
-    (s) => s["@_href"] !== `#${decisionId}`
-  );
+  const xmlHref = buildXmlHref({ namespace: href.namespace, id: href.id });


The same here. xmlHref will not have the same value as the decisionHref?

Same answer as above.

xmlHref is exactly the same as href... you're even building it with the same values. No need to have this xmlHref variable...

ljmotta · 2024-09-09T12:44:05Z

packages/dmn-editor/src/mutations/addDecisionToDecisionService.ts

+    if (decision?.__$$element !== "decision") {
+      throw new Error(
+        `DMN MUTATION: DRG Element with id '${href.id}' is either not a Decision or doesn't exist in the external model '${href.namespace}'`
+      );
+    }


The error message is targeting two different cases. Not having an element and the element not being a Decision are two distinguish scenarios. I think this should be break into two errors. If decision === undefined the element doesn't exist message. And the decision.__$$element !== "decision" the error telling isn't a Decision. WDYT?

I'm following the existing error message patterns used in this mutation and in deleteDecisionFromDecisionService. In my opinion, we will only be adding more code to do something that we can do in fewer lines.

ljmotta · 2024-09-09T12:44:30Z

packages/dmn-editor/src/mutations/deleteDecisionFromDecisionService.ts

+    if (decision?.__$$element !== "decision") {
+      throw new Error(
+        `DMN MUTATION: DRG Element with id '${href.id}' is either not a Decision or doesn't exist in the external model '${href.namespace}'`
+      );
+    }


The same with this error.

ljmotta · 2024-09-09T12:56:57Z

packages/dmn-editor/src/mutations/repopulateInputDataAndDecisionsOnDecisionService.ts

+  for (const hrefString of hrefsToDecisionsInsideDecisionService) {
+    const href = parseXmlHref(hrefString);
+    const externalModel = externalModelsByNamespace?.[href.namespace ?? ""];
+    if (externalModel) {
+      const externalDecision = (externalModel?.model as Normalized<DmnLatestModel>).definitions.drgElement?.find(
+        (drgElement) => drgElement["@_id"] === href.id
+      ) as Normalized<DMN15__tDecision>;
+
+      if (externalDecision) {
+        (externalDecision.informationRequirement ?? []).flatMap((ir) => {
+          if (ir.requiredDecision) {
+            const externalHref = parseXmlHref(ir.requiredDecision["@_href"]);
+            // If the requiredDecision has namespace, it means that it is pointing to a node in a 3rd model,
+            // not this one (the local model) neither the model in the `href.namespace`.
+            if (externalHref.namespace) {
+              requirements.set(`${ir.requiredDecision["@_href"]}`, "decisionIr");
+            } else {
+              requirements.set(`${href.namespace}${ir.requiredDecision["@_href"]}`, "decisionIr");
+            }
+          } else if (ir.requiredInput) {
+            // If the requiredInput has namespace, it means that it is pointing to a node in a 3rd model,
+            // not this one (the local model) neither the model in the `href.namespace`.
+            const externalHref = parseXmlHref(ir.requiredInput["@_href"]);
+            if (externalHref.namespace) {
+              requirements.set(`${ir.requiredInput["@_href"]}`, "inputDataIr");
+            } else {
+              requirements.set(`${href.namespace}${ir.requiredInput["@_href"]}`, "inputDataIr");
+            }
+          } else {
+            throw new Error(
+              `DMN MUTATION: Invalid information requirement referenced by external DecisionService: '${externalDecision["@_id"]}'`
+            );
+          }
+        });
+      }
+    }
+  }


Adding just a suggestion here. The usage of guard clauses to reduce nesting:

if (externalModel === undefined) { continue; } const externalDecision = (externalModel.model as Normalized<DmnLatestModel>).definitions.drgElement?.find((drgElement) => drgElement["@_id"] === href.id) as Normalized<DMN15__tDecision>; if (externalDecision === undefined) { continue; } ...

Also, why did you used the flatMap instead of forEach?

The code with flatMap is a copy-paste of the same code a few lines above, but it uses the drgElement and has a simpler logic. Do you want me to change both?

Good point (the nesting)!

Yes, please. It doesn't make sense to use flatMap if we're not retuning the value.

ljmotta · 2024-09-09T13:35:22Z

packages/dmn-editor/tests-e2e/__fixtures__/base.ts

+  stories: async ({ baseURL, page }, use) => {
+    await use(new Stories(page, baseURL));
+  },
+  includedModels: async ({ baseURL, page }, use) => {


Unused baseUrl

danielzhe · 2024-09-09T15:18:31Z

I've made some comments inline. One tip regarding objects. When the object property and value has the same name you could short it just the property name:

I'm aware of this. If I didn't do it probably I just overlooked it or it was something that was renamed. 😄 @ljmotta

I'm checking the other things you reported. Thank you.

danielzhe · 2024-09-09T18:18:18Z

Changes, applied, @ljmotta

packages/dmn-editor/src/externalNodes/DmnObjectListItem.tsx

tiagobento · 2024-09-10T14:42:27Z

packages/dmn-editor/src/includedModels/IncludedModels.tsx

@@ -471,6 +472,7 @@ function IncludedModelCard({
          definitions: state.dmn.model.definitions,
          __readonly_index: index,
          __readonly_externalModelTypesByNamespace: externalModelTypesByNamespace,
+          externalModelsByNamespace,


__readonly_externalModelsByNamespace

tiagobento · 2024-09-10T14:42:42Z

packages/dmn-editor/src/diagram/Diagram.tsx

@@ -876,6 +879,7 @@ export const Diagram = React.forwardRef<DiagramRef, { container: React.RefObject
                  __readonly_externalModelTypesByNamespace: state
                    .computed(state)
                    .getExternalModelTypesByNamespace(externalModelsByNamespace),
+                  externalModelsByNamespace,


__readonly_externalModelsByNamespace

packages/dmn-editor/src/diagram/Diagram.tsx

tiagobento · 2024-09-10T16:36:07Z

packages/dmn-editor/src/mutations/deleteDecisionFromDecisionService.ts

-  decisionService.outputDecision = (decisionService.outputDecision ?? []).filter(
-    (s) => s["@_href"] !== `#${decisionId}`
-  );
+  const xmlHref = buildXmlHref({ namespace: href.namespace, id: href.id });


xmlHref is exactly the same as href... you're even building it with the same values. No need to have this xmlHref variable...

packages/dmn-editor/src/mutations/repopulateInputDataAndDecisionsOnDecisionService.ts

packages/dmn-editor-envelope/src/DmnEditorRoot.tsx

tiagobento · 2024-09-10T16:44:14Z

packages/dmn-editor-envelope/src/DmnEditorRoot.tsx

+          loadDependentModels(
+            includedModel,
+            externalModelsIndex,
+            resourcesByNamespace,
+            loadedDmnsByPathRelativeToTheWorkspaceRoot,
+            thisDmnsNormalizedPosixPathRelativeToTheWorkspaceRoot
+          );


This useCallback depends on itself.. This is not very good I'd say. Not even sure how ESLint didn't complain about this, as you're inhenritenly depending on loadDependentModels from a previous tick (I.e., outdated getIncludedNamespacesFromModel).

…niel-tiago .

ljmotta

Great changes, the rename helped a lot. I've made some comments inline regarding error messages and minor things.

ljmotta · 2024-09-18T18:30:58Z

packages/dmn-editor/src/mutations/addDecisionToDecisionService.ts

@@ -54,8 +77,11 @@ export function addDecisionToDecisionService({
  }

  const diagram = addOrGetDrd({ definitions, drdIndex });
+  const hrefString = buildXmlHref({ namespace: href.namespace, id: href.id });


@danielzhe @tiagobento how are we going to proceed here?

ljmotta · 2024-09-18T18:33:25Z

packages/dmn-editor/src/mutations/deleteDecisionFromDecisionService.ts

@@ -20,21 +20,45 @@
 import { DMN15__tDefinitions } from "@kie-tools/dmn-marshaller/dist/schemas/dmn-1_5/ts-gen/types";
 import { repopulateInputDataAndDecisionsOnDecisionService } from "./repopulateInputDataAndDecisionsOnDecisionService";
 import { Normalized } from "../normalization/normalize";
+import { buildXmlHref, parseXmlHref } from "../xml/xmlHrefs";


unused import

packages/dmn-editor/src/mutations/deleteImport.ts

ljmotta · 2024-09-18T19:03:00Z

packages/dmn-editor/src/externalNodes/DmnObjectListItem.tsx

+
+  const nodeTypeTooltipDescription = useMemo(() => {
+    if (dmnObject === undefined) {
+      throw new Error("nodeTypeDescription can't be defined without a DMN object");


The nodeTypeDescription and DMN Object doesn't make much sense in the error message. The dmnObject is a drgElement, I'm not sure about the context behind this name, maybe @tiagobento could help us here. "Can't define the tooltip description without a DRG element" WDYT?

Yeah, the name dmnObject is definitely sub-optimal. That's on me, though. We just need to be careful to not crash the editor over an error like this. Let's define to the "Unknown" string and remove both throws please.

@danielzhe

ljmotta · 2024-09-18T19:04:19Z

packages/dmn-editor/src/externalNodes/DmnObjectListItem.tsx

+    }
+    const nodeType = getNodeTypeFromDmnObject(dmnObject);
+    if (nodeType === undefined) {
+      throw new Error("Can't determine nodeTypeDescription with undefined node type");


The same here. "Can't define the tooltip description without the node type" WDYT?

Same as above. @danielzhe

ljmotta · 2024-09-19T00:31:57Z

I found some interesting cases doing some manual checkings:

We have here three DRDs, in DRD3 we can observe that the ds2 is in collapsed form, and d3 isn't available to pick from the node palette, as the node was depict.

Dragging a node near the collapse DS will trigger the invalid operation in the DS.

It's possible to hide external Decisions that are part of a Decision Service. Adding them back to the DRD will not remove them from the DS, resulting some weird behaviors

Recording.2024-09-18.212657.mp4

A DRD that has Decision nodes that are part of a collapsed Decision Service can't be moved:

Recording.2024-09-18.212950.mp4

Co-authored-by: Luiz João Motta <[email protected]>

tiagobento · 2024-09-19T13:36:35Z

@ljmotta Can you please discriminate between what's already on main and what's been introduced by this PR? Some of those bugs (E.g., Dragging a node near the collapse DS will trigger the invalid operation in the DS.) you reported seem not to be related to the changes this PR introduces.

tiagobento · 2024-09-19T13:47:57Z

@danielzhe I would double-check those too, just to be sure.

ljmotta · 2024-09-19T13:48:07Z

@tiagobento Both, 1 and 2 are present on main. I could reproduce using the daily-dev.

danielzhe · 2024-09-26T20:52:40Z

Recording.2024-09-26.175024.mp4

danielzhe · 2024-09-26T20:56:33Z

moving.tests.mp4

danielzhe added 2 commits August 1, 2024 16:42

kie-issues#1298: Decision Services & multiple DRDs: make it possible …

b690ebb

…to add external Decisions to a Decision Service.

Tests e2e

33de8ea

danielzhe added the area:dmn label Aug 2, 2024

danielzhe requested a review from tiagobento as a code owner August 2, 2024 22:07

danielzhe commented Aug 2, 2024

View reviewed changes

webkit

2db60f6

danielzhe force-pushed the included-ds branch from 077f040 to 2db60f6 Compare August 2, 2024 22:19

danielzhe requested a review from jomarko August 5, 2024 13:12

tiagobento requested changes Aug 5, 2024

View reviewed changes

danielzhe added 3 commits August 5, 2024 16:55

Code review. More tests. Fixed an issue about moving nodes from DS to…

b55113d

… outside.

Tests screenshots 1

24f1b15

Tests screenshots 2

f916099

danielzhe force-pushed the included-ds branch from 2e9b354 to 512a274 Compare August 6, 2024 01:07

Tests screenshots 3

99924ea

danielzhe force-pushed the included-ds branch from 512a274 to 99924ea Compare August 6, 2024 01:31

jomarko reviewed Aug 6, 2024

View reviewed changes

danielzhe and others added 2 commits August 6, 2024 11:15

Better screenshots for test.

d64a04e

Mutations now does not count with "correct parameters".

a078041

data-testid renamed. General code review. Typo fix.

danielzhe commented Aug 6, 2024

View reviewed changes

packages/dmn-editor/src/externalNodes/ExternalNodesPanel.tsx Show resolved Hide resolved

danielzhe commented Aug 6, 2024

View reviewed changes

danielzhe added 2 commits August 7, 2024 03:28

Href.

c9f88bf

Fixing tests.

Merge branch 'main' into included-ds

411e61d

tiagobento requested changes Aug 7, 2024

View reviewed changes

danielzhe and others added 3 commits August 7, 2024 17:53

Update packages/dmn-editor/src/mutations/addDecisionToDecisionService.ts

6cd536f

Co-authored-by: Tiago Bento <[email protected]>

Update packages/dmn-editor/src/diagram/Diagram.tsx

a44b7da

Co-authored-by: Tiago Bento <[email protected]>

Update packages/dmn-editor/src/mutations/deleteDecisionFromDecisionSe…

fad6167

…rvice.ts Co-authored-by: Tiago Bento <[email protected]>

Fixing hooks and renaming variable

8bbb1cf

ljmotta reviewed Sep 9, 2024

View reviewed changes

Reduce nesting. Remove unused parameters.

81d260f

tiagobento requested changes Sep 10, 2024

View reviewed changes

tiagobento and others added 7 commits September 11, 2024 10:59

.

1bdc614

Merge pull request #8 from tiagobento/included-ds-pair-programming-da…

ffa1867

…niel-tiago .

Merge branch 'main' into included-ds

2921171

Refactor DmnEditorRoot

e22fcd1

Refactor DmnEditorRoot

3cb35a5

Clean

334cf35

clean

bd5786c

ljmotta reviewed Sep 18, 2024

View reviewed changes

Update packages/dmn-editor/src/mutations/deleteImport.ts

0e90a76

Co-authored-by: Luiz João Motta <[email protected]>

danielzhe added 5 commits September 25, 2024 11:13

Fixes issues 3 and 4

a03e182

decisionHref

d9408e7

hook

4759ae3

Revert

91a815f

Fix 3 and 4 issues

702f44f

tiagobento approved these changes Sep 27, 2024

View reviewed changes

tiagobento requested a review from jomarko September 27, 2024 15:35

		import { useCallback, useMemo, useRef, useState } from "react";
		import type { Meta, StoryObj } from "@storybook/react";

		decisionShape: selectedNodes[i].data.shape,
		elementId: selectedNodes[i].id, // The "real" id is here, which can be the local id or an imported node id (uri + external element id).

		await includedModels.fillModelToInclude({ modelName: "sumDiffDs.dmn" });
		await includedModels.selectModel({ modelName: "sumDiffDs.dmn" });

		const externalDrgs = (externalModel.model as Normalized<DmnLatestModel>).definitions.drgElement;
		const decision = externalDrgs?.find((drgElement) => drgElement["@_id"] === href.id);

kie-issues#1298: Decision Services & multiple DRDs: make it possible o add external Decisions to a Decision Service #2508

Are you sure you want to change the base?

kie-issues#1298: Decision Services & multiple DRDs: make it possible o add external Decisions to a Decision Service #2508

Conversation

danielzhe commented Aug 2, 2024

danielzhe commented Aug 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiagobento left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jomarko commented Aug 5, 2024

jomarko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiagobento Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielzhe commented Aug 6, 2024

tiagobento left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielzhe commented Sep 6, 2024

jomarko commented Sep 9, 2024

ljmotta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiagobento Aug 6, 2024 •

edited

Loading